Repository navigation
Keep a parenthesized HQL scalar query from being typed as the domain class - #16719
jamesfredley wants to merge 3 commits into
Conversation
…class Unwrap only parentheses that enclose the whole query when choosing the result type. The HQL sent to Hibernate is unchanged, and a nested subquery is still left intact. Fixes #16712 Assisted-by: Sisyphus:grok-4.7
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
unwrapRootQueryrelies onSqlRestriction.skipQuotedto always advancei. IfskipQuotedever… · New/* ... */) but not end-of-line SQL/HQL comments… · New), but the new scanning… · New
What changed in this PR
Fixes incorrect result type inference for parenthesized root HQL scalar queries (e.g., (select b.title ...)) by unwrapping only parentheses that enclose the entire query during analysis, while leaving executed HQL unchanged.
Changes:
- Adjusted HQL target-type inference to preprocess queries via a new root-parentheses unwrapping step.
- Added coverage for scalar, entity, aggregate, and tuple results when the root query is parenthesized.
| File | Description |
|---|---|
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlQueryContext.java | Adds unwrapRootQuery and uses it in result-type inference to avoid treating parenthesized root queries as subqueries. |
| grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/HqlSelectResultTypeSpec.groovy | Adds regression tests for parenthesized root query result-type inference across scalar/entity/aggregate/tuple projections. |
Review effort: Lite
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16719 +/- ##
==============================
==============================
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
The fix is right for the query the issue opens with: unwrapRootQuery strips only parentheses that enclose the whole query, for analysis only, skips literals and block comments the way indexOfTopLevel does, and leaves a nested subquery intact. The cases in HqlSelectResultTypeSpec are well chosen, and Codecov shows every changed line covered. The three red checks are the Groovy Snapshot Canary (Fongo/Mongo in the mongodb example apps), not this change.
Two things before this merges:
-
The parenthesized
UNIONbranches from #16712 are still broken. The issue says "a fully parenthesized scalarUNIONbranch has the same problem", and this fix cannot reach it, because the parentheses do not enclose the whole query. On this branch:HqlResultTypeBook.executeQuery('select b.title from HqlResultTypeBook b union select a.name from HqlResultTypeAuthor a') // passes HqlResultTypeBook.executeQuery('(select b.title from HqlResultTypeBook b) union (select a.name from HqlResultTypeAuthor a)') // fails
The second one fails with
QueryTypeMismatchException: query produces 'java.lang.String' but type 'grails....HqlResultTypeBook' was specified. Analyse the first branch when the query starts with a parenthesized group followed by a top-levelunion,intersectorexcept, and add that row to the spec. -
Please build this on the classes of #16616 instead of adding a fourth scanner to the record.
unwrapRootQueryis one moreprivate staticmethod that walks parentheses and skips comments with its own rules, next toindexOfTopLevel,countHqlProjectionsandgetSingleProjectionClause, which is the shape #16616 asks to leave behind. StartHqlTexthere: the HQL string with the scanning in one place (the root-query unwrapping, the top-levelselect/fromsearch, the projection count and the result type, found once), and haveHqlQueryContextdelegate to it.HqlTextSpecthen takes the cases of this PR as awhere:table: whole-query parentheses, the union branches above, literals and quoted identifiers containing), block comments, and the--line comment Copilot raised, which neither scanner handles today. A scanner with its own spec would have made the union case hard to miss, and it makes the next case cheap to add.That moves the start of #16616 from 9.0.x to this branch; #16572 then carries the split up as part of its merge-up, which is fine.
QueryParametersandQuerySettingscan follow separately.
On Copilot's three comments: the skipQuoted non-progress guard is not needed, it always advances at least one character; the -- comment is the real one and belongs in HqlText; backticks are already covered by skipQuoted.
|
Update on point 2: rather than ask you to start |
|
The shared scanner is up as #16735 ( |
…Projection alias and a bracket identifier with an apostrophe working Fixes apache#16717. Starts apache#16616 with its step 0. Three scanners in org.grails.orm.hibernate.query each decided on their own what to skip: HqlQueryContext.indexOfTopLevel and getCommas, SqlRestriction's placeholder scan, and SqlProjection.splitColumns. Every recent bug of this family was a rule mismatch between them (apache#16597, apache#16712, apache#16717). SqlText now owns the rules once and reads the text once: when an instance is created, each character is classified as plain text, quoted text or comment, and each plain character gets its parenthesis depth, so every later question (indexOfTopLevel, splitTopLevel, countTopLevel, plainIndexes, unwrapOuterParentheses, withoutTrailingComments, endsInLineComment) walks the two arrays and nothing rescans the string. The rules: string literals and quoted identifiers with a doubled quote for the quote; in native SQL also [bracket identifiers] with ]] and dollar-quoted strings; line and block comments; parentheses for a top-level position. The three scanners delegate to it, and HqlQueryContext creates one instance per operation instead of lowercasing and rescanning for the select, the from and the commas. The two defects of apache#16717: - A sqlProjection alias with a space broke a count over the projection: JpaProjectionAdapter gave a subquery selection a generated alias only when it had none, so the derived table of `createCriteria().count { projections { sqlProjection ... } }` was rendered as derived1_0(box width), which H2 rejects. Nothing reads a subquery's selections by alias, so every subquery selection now gets a generated one; the main query keeps the caller's alias. - An apostrophe inside a bracket identifier swallowed the rest of the SQL, because quotes were tested before the bracket state. Inside brackets only brackets count now. Two behaviours improve as a side effect: the HQL projection count ignores a comma inside a comment or a backquoted identifier, and the HQL keyword search skips line comments. unwrapOuterParentheses is in place for apache#16719. SqlTextSpec holds one table per capability; SqlProjectionSpec and HqlQueryContextSpec gain the rows the fix adds; the sqlProjection integration spec gains the count over a spaced alias, which fails before the fix.
Move the scan into HqlText so a leading parenthesized union, intersect, or except is typed from that branch. The executed query is unchanged. Adjacent minus tokens stay arithmetic, and block comments still hide parentheses. Assisted-by: sisyphus-cursor-grok:grok-4.7
|
Follow-up is A leading parenthesized group followed by a top-level union, intersect, or except is analyzed from the first branch. The scan lives in
|
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Round 2 at bac0c0a. The follow-up does what the first review asked in substance: the first branch of a leading parenthesized group is analyzed, (select b.title from HqlResultTypeBook b) union (select a.name from HqlResultTypeAuthor a) now returns Strings, the HQL sent to Hibernate is untouched, and -- is read as subtraction, which is what Hibernate's lexer does (7.4.13 has one COMMENT token, the block comment) and what HqlCommentsSpec on 8.0.x already pins. HqlTextSpec (32), HqlQueryContextSpec (63) and HqlSelectResultTypeSpec (12) pass on this head.
It cannot merge as it stands, for one reason: the branch is still on 75a355a, before #16735 landed on 8.0.x on 10-24, and it adds a second HqlText. GitHub shows it conflicting: add/add on HqlText.java and HqlTextSpec.groovy, content on HqlQueryContext.java. #16735 did not fix #16712: on current 8.0.x every parenthesized row of your HqlSelectResultTypeSpec still fails with the same type mismatch, so this fix is still needed, and it gets smaller after the rebase. The scanning move is already there; what remains is the root unwrap and the first-branch rule inside the existing HqlText, plus your rows. Details inline.
One gap carried over from round 1, inline: a leading group followed by limit, or by any clause other than a set operator, still gets the entity type.
Two small things: pages on the fixture is not read by any executed query, and the three replies to Copilot came through as file paths (@C:\Users\james\...\reply-hql-skip.txt) rather than their text, so please repost them.
| '(/* ignore ) and ( select x from y */ select b.title from Book b)' | 1 | 'b.title' | Object | ||
| '(/* ignore ) and ( select x from y\n */ select b.title from Book b)' | 1 | 'b.title' | Object | ||
| '(select b.title /* ), ignored */ from Book b)' | 1 | 'b.title /* ), ignored */' | Object | ||
| '(select b.pages--1 from Book b)' | 1 | 'b.pages--1' | Object |
There was a problem hiding this comment.
Already pinned on 8.0.x: HqlCommentsSpec runs Hibernate's parser on b.height--1, the merged HqlTextSpec has the b.height--1 > 0 select b.width row, and SqlSyntax.HQL has no line comments. Fine to keep as the parenthesized variant after the rebase.
The group supplies the projection count and result type. The query sent to Hibernate stays as written, so an unparenthesized select in a later union arm is not given the synthetic alias. This also merges current 8.0.x. Assisted-by: Sisyphus:grok-4.7
✅ All tests passed ✅🏷️ Commit: b7bb164 Learn more about TestLens at testlens.app/docs. |
Fixes #16712.
A parenthesized root query such as
(select b.title from Book b)was treated as a subquery when choosing the result type, soexecuteQuerywas given the domain class and failed. The result type is now read from a leading parenthesized group, whatever follows it. The HQL sent to Hibernate stays the whole original text, including an unparenthesized select in a later union arm.That reading lives in the existing
HqlTexton 8.0.x.HqlQueryContextstill sendsnormalized()to Hibernate.--is not treated as a line comment. Hibernate parsesb.pages--1as subtraction followed by unary minus, andHqlTextSpeclocks that projection as a scalar. Executing that form on H2 fails later because the generated SQL containspages--1, which H2 reads as a comment.